Fix output image consistency issue #366

takahirom · 2024-05-18T03:34:39Z

It seems that the task information provided by TestTaskSkipEventsServiceProvider, which is necessary to decide whether we should restore the cache to the output directory, isn't saved correctly. Therefore, I removed TestTaskSkipEventsServiceProvider and now always restore the cache. The performance impact appears to be minimal, and consistency is more important.

takahirom · 2024-05-18T03:53:01Z

The reason why the test fails is that when the test fails, testTask.doLast{} is not executed, so intermediateDir is not updated.

  @Test
  fun verifyAndRecord_changeDetect() {
    RoborazziGradleProject(testProjectDir).apply {
      record()
      val recordFileHash1 = getFileHash("$screenshotAndName.testCapture.png")
      changeScreen()

      verifyAndRecordAndFail()

      val recordFileHash2 = getFileHash("$screenshotAndName.testCapture.png")
      assert(recordFileHash1 != recordFileHash2)

takahirom · 2024-05-18T05:44:00Z

@lukas-mercari
I think it is better to merge this fix, but we aren't able to reproduce the problem in our repository. Could you take a look?

lukas-mercari · 2024-05-23T13:04:12Z

@takahirom I believe the requirement to reproduce this is to make sure the task is taken from both configuration and build cache. (I can reproduce in the same way that I reproduced #374.)

Build cache: Because it means that the tests will be skipped.
Configuration cache: In this case the skip detection will not work properly because expectingTestNames is not initialised correctly.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [io.github.takahirom.roborazzi](https://togithub.com/takahirom/roborazzi) | plugin | minor | `1.16.1` -> `1.17.0` | | [io.github.takahirom.roborazzi:roborazzi-junit-rule](https://togithub.com/takahirom/roborazzi) | dependencies | minor | `1.16.1` -> `1.17.0` | | [io.github.takahirom.roborazzi:roborazzi-compose](https://togithub.com/takahirom/roborazzi) | dependencies | minor | `1.16.1` -> `1.17.0` | | [io.github.takahirom.roborazzi:roborazzi](https://togithub.com/takahirom/roborazzi) | dependencies | minor | `1.16.1` -> `1.17.0` | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>takahirom/roborazzi (io.github.takahirom.roborazzi)</summary> ### [`v1.17.0`](https://togithub.com/takahirom/roborazzi/releases/tag/1.17.0) [Compare Source](https://togithub.com/takahirom/roborazzi/compare/1.16.1...1.17.0) ##### Bug Fixes - There seemed to be a pattern where ActivityScenario's close was not being called when tests failed, causing leaks, so I used a use block to ensure it is closed properly. [@yschimke](https://togithub.com/yschimke), thank you for pointing out this issue. - At companies that use Roborazzi on a large scale, there were issues with the output dir not being restored properly due to Configuration cache and other factors, so I simplified the design to make problems less likely to occur. - There were occasional errors reporting that a directory did not exist, and I've added code to address this issue. [@lukas-mercari](https://togithub.com/lukas-mercari), thank you for your contribution. ##### What's Changed - Use the `use` block to prevent leaks by [@takahirom](https://togithub.com/takahirom) in [https://github.com/takahirom/roborazzi/pull/376](https://togithub.com/takahirom/roborazzi/pull/376) - Declare input directories as files to avoid issues when they don't exist by [@lukas-mercari](https://togithub.com/lukas-mercari) in [https://github.com/takahirom/roborazzi/pull/374](https://togithub.com/takahirom/roborazzi/pull/374) - Fix output image consistency issue by [@takahirom](https://togithub.com/takahirom) in [https://github.com/takahirom/roborazzi/pull/366](https://togithub.com/takahirom/roborazzi/pull/366) - \[CI] Update toolchain to Java 17 for all modules by [@mannodermaus](https://togithub.com/mannodermaus) in [https://github.com/takahirom/roborazzi/pull/365](https://togithub.com/takahirom/roborazzi/pull/365) . Thank you for managing our build process, [@mannodermaus](https://togithub.com/mannodermaus)! - \[Sample] fix(deps): update dependency androidx.compose.material:material to v1.6.7 by [@renovate](https://togithub.com/renovate) in [https://github.com/takahirom/roborazzi/pull/342](https://togithub.com/takahirom/roborazzi/pull/342) - fix(deps): update dependency org.robolectric:robolectric to v4.12.2 by [@renovate](https://togithub.com/renovate) in [https://github.com/takahirom/roborazzi/pull/369](https://togithub.com/takahirom/roborazzi/pull/369) - \[CI] Exclude `io.github.takahirom.roborazzi` dependency from Renovate updates by [@sanao1006](https://togithub.com/sanao1006) in [https://github.com/takahirom/roborazzi/pull/372](https://togithub.com/takahirom/roborazzi/pull/372) ##### New Contributors - [@mannodermaus](https://togithub.com/mannodermaus) made their first contribution in [https://github.com/takahirom/roborazzi/pull/365](https://togithub.com/takahirom/roborazzi/pull/365) **Full Changelog**: takahirom/roborazzi@1.16.1...1.17.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate).

lukas-mercari · 2024-05-27T09:23:34Z

@takahirom I upgraded to the latest version in our CI/CD pipeline, but it seems that sometimes we get the following exception:

...
Caused by: java.io.IOException: Source file wasn't copied completely, length of destination file differs.
	at kotlin.io.FilesKt__UtilsKt.copyRecursively(Utils.kt:329)
	at kotlin.io.FilesKt__UtilsKt.copyRecursively$default(Utils.kt:288)
	at io.github.takahirom.roborazzi.RoborazziPlugin$apply$configureRoborazziTasks$6$8.afterSuite(RoborazziPlugin.kt:366)
	at jdk.internal.reflect.GeneratedMethodAccessor1126.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	... 44 more

My hypothesis is that it might happen that while one suite finished (and afterSuite is called), another is still generating images -- and thus files in the intermediate dir keep changing.

takahirom · 2024-05-27T10:22:37Z

👀

takahirom · 2024-05-27T10:24:28Z

@lukas-mercari
Thanks. I saw a similar issue. Are you running a single variant?
#377 (comment)

lukas-mercari · 2024-05-27T10:57:43Z

@takahirom Yes, we only run a single variant per runner. (E.g. ./gradlew recordRoborazziDebug)

takahirom · 2024-05-27T11:16:29Z

@lukas-mercari
If your hypothesis is correct, we might have to check if it is the root suite. Are you seeing multiple 'Roborazzi: test.doLast Copy files' logs?
https://stackoverflow.com/a/68518970

takahirom · 2024-05-27T11:25:45Z

In any case, we might need to include this if statement.
https://github.com/search?q=%22suite.parent+%3D%3D+null%22&ref=opensearch&type=code

takahirom · 2024-05-27T11:34:41Z

@lukas-mercari
Could you please take a look at this PR?
https://github.com/takahirom/roborazzi/pull/385/files

lukas-mercari · 2024-05-27T11:36:54Z

Are you seeing multiple 'Roborazzi: test.doLast Copy files' logs?

Yes, I can see multiple logs (when running recording :sample-android:recordRoborazziDebug). Confirmed that the approach to find the root test suite seems to work!

Check integrationt tests when removing isskiptest in Plugin

a4b20de

takahirom changed the title ~~Check integrationt tests when removing isskiptest in Plugin~~ Check integration tests when removing isskiptest in Plugin May 18, 2024

takahirom mentioned this pull request May 18, 2024

[WIP] Cleanup intermediate dir usage, since there seem to be some problems restoring it when caches are used #363

Closed

takahirom added 4 commits May 18, 2024 13:08

Try to use TestListener instead of doLast of task

75a1380

Remove isTestSkipped completely

d984b7f

Add comments

d84be71

Remove some comments

5392f08

takahirom changed the title ~~Check integration tests when removing isskiptest in Plugin~~ Fix output image consistency issue May 18, 2024

lukas-mercari mentioned this pull request May 20, 2024

Declare input directories as files to avoid issues when they don't exist #374

Merged

takahirom merged commit c0d9101 into main May 24, 2024
5 checks passed

takahirom deleted the takahirom/check-integration-tests-when-removing-isskiptest/2024-05-18 branch May 24, 2024 00:54

lukas-mercari mentioned this pull request May 27, 2024

Fix issue where afterSuite runs for non-root suites #385

Merged

renovate bot mentioned this pull request Nov 7, 2024

Update roborazzi to v1.32.1 dao0203/todo-sample-app#36

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix output image consistency issue #366

Fix output image consistency issue #366

takahirom commented May 18, 2024 •

edited

Loading

takahirom commented May 18, 2024 •

edited

Loading

takahirom commented May 18, 2024

lukas-mercari commented May 23, 2024 •

edited

Loading

lukas-mercari commented May 27, 2024

takahirom commented May 27, 2024

takahirom commented May 27, 2024

lukas-mercari commented May 27, 2024

takahirom commented May 27, 2024

takahirom commented May 27, 2024

takahirom commented May 27, 2024

lukas-mercari commented May 27, 2024

Fix output image consistency issue #366

Fix output image consistency issue #366

Conversation

takahirom commented May 18, 2024 • edited Loading

takahirom commented May 18, 2024 • edited Loading

takahirom commented May 18, 2024

lukas-mercari commented May 23, 2024 • edited Loading

lukas-mercari commented May 27, 2024

takahirom commented May 27, 2024

takahirom commented May 27, 2024

lukas-mercari commented May 27, 2024

takahirom commented May 27, 2024

takahirom commented May 27, 2024

takahirom commented May 27, 2024

lukas-mercari commented May 27, 2024

takahirom commented May 18, 2024 •

edited

Loading

takahirom commented May 18, 2024 •

edited

Loading

lukas-mercari commented May 23, 2024 •

edited

Loading